Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/in order notifications cleanup #13

Merged
merged 3 commits into from
Jun 11, 2020

Conversation

hannahhoward
Copy link
Contributor

@hannahhoward hannahhoward commented Jun 10, 2020

Goals

A bit of cleanup to solve two different issues here:

  1. Make sure go routines get cleaned up
  2. Make sure state machine doesn't block on channel write if notification handling is slow (this was an outstanding issue we wanted to solve) -- this uses a technique for unbounded channels described here: https://medium.com/capital-one-tech/building-an-unbounded-channel-in-go-789e175cd2cd
  3. Make sure go routine doesn't start till events are sent.
  4. Move support for a seperate "init" function into core state machine functionality as optional config.

@ingar
Copy link
Contributor

ingar commented Jun 10, 2020

Just wanted to clarify the code regarding regarding:

Make sure state machine doesn't block on channel write if notification handling is slow

The double channel and unbounded slice solution solves this, because if we ever hit the capacity of our inbound notification channel it would block.

For our use cases we would currently never come close to hitting the queue limit given our state machine configurations, and we could easily increase the channel buffer to 1k or more.

But this unbounded solution takes care of it in all cases. I am just wondering about the complexity tradeoff.

@hannahhoward
Copy link
Contributor Author

@ingar yes... though, keep in mind that single channel is for all deals, so maybe? I dunno. I'm always hesistant to use channel buffers to make a channel into effectively an asynchronous mailbox. but that could be my neurosis.

@ingar
Copy link
Contributor

ingar commented Jun 11, 2020

Oh right, for all deals. That makes way more sense.

@ingar ingar merged commit c290494 into fix/in-order-notifications Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants